feat(platform): add typed refund owners to storage flags and fee refunds - #4789
DCG-Claude wants to merge 10 commits into
Conversation
Storage refunds were keyed by a bare 32-byte identifier that was always read as an identity. Contract credit buckets can also pay for storage, so the owner of stored bytes is now a typed RefundOwner (Identity or ContractBucket) with a deterministic carrier key: an identity keeps its id, a bucket derives a domain separated double SHA-256 of contract id and position. FeeRefunds keeps the historical carrier as its first field and records the typed owner of every carrier key in a second field; the untyped constructor records identities explicitly, the typed constructor reads the recorded map and rejects an unmapped key, and merging rejects one key that names two owners. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The storage flags module stops re-exporting the grovedb crate type and defines Drive's own StorageFlags. The four historical variants keep their names and shapes and are encoded and decoded through the pinned crate so their bytes cannot drift. Two variants are added for bytes a contract credit bucket paid for: type byte 4 (single epoch, 37 bytes) and type byte 5 (multi epoch). The parse helpers decode all six types so the shipped readers carry bucket flags through; the shipped-name batch closure entry points hand raw bytes to the crate and therefore reject the bucket types, so the shipped batch apply generation fails closed. The typed closure entry points section removed bytes under the owner's carrier key and record the owner, refusing one key that names two owners. Combine maps typed owners to their carrier keys for the crate's epoch arithmetic and maps the winner back through the inputs, so a replace with UseTheirs transfers ownership across kinds. The eight direct crate imports switch to the Drive path. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds LowLevelDriveOperation::CalculatedCostOperationWithRefundOwners, a measured cost whose sectioned storage removal comes with the typed owner recorded for every carrier key. operation_cost rejects it so the shipped fee decoder fails closed on it through its catch-all arm; combine_cost_operations sums it. push_drive_operation_result_with_refund_owners pushes it. grove_apply_batch_with_add_costs and grove_apply_partial_batch_with_add_costs gain a version 1 that splits removed bytes with the typed storage flags and pushes the typed cost operation. No version table selects them yet: the dispatcher tests select version 1 through a test-built drive version and pin that an identity-owned delete yields the same root hash and removed bytes under both generations, that a bucket-owned delete records its owner under version 1, and that the full batch version 0 rejects bucket flags. The shipped partial batch generation inlines its closures and therefore sections bucket bytes without recording an owner; the test pins that too, with the reasoning for why it is acceptable (no production caller, bucket bytes exist only from the version that selects the typed generation). Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e book Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…gs type Replacing the storage flags re-export with a Drive type silently rebound both shipped batch apply generations through their unchanged import line. The partial batch v0 inlines its closures over the parse helpers, which decode the contract bucket flag types, so over bucket flags it sectioned the bytes under the carrier key and pushed a plain cost operation with no recorded owner; the shipped fee decoder would then have read that key as an identity and credited the wrong account. Both v0 modules now import the crate's StorageFlags directly, the exact type they were compiled against before, so their behaviour is identical to what shipped and they reject type bytes 4 and 5 through the crate's decoder. The Drive type drops the delegating shipped-name closure wrappers, which nothing else called. Dispatcher tests on both methods pin that a bucket-owned delete under generation 0 errors, applies nothing and prices no removal, and that generation 1 then removes it with the recorded owner. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-18T09:35:50.062Z |
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v5.0-dev #4789 +/- ##
=============================================
- Coverage 86.36% 76.19% -10.18%
=============================================
Files 2766 2806 +40
Lines 366105 410674 +44569
=============================================
- Hits 316191 312917 -3274
- Misses 49914 97757 +47843
🚀 New features to boost your workflow:
|
|
Codecov note for reviewers, so nobody chases the red The project drop is a runner-side mapping artifact, not lost coverage. The head report maps line tables that do not belong to this branch:
🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the findings against head 0de766d. Shipped batch generations remain unchanged, but a dispatcher-level probe confirmed that the new typed generation retains the old bucket owner during a later-epoch shrinking replacement instead of honoring UseTheirs. The 115 targeted Drive tests and 37 existing DPP refund tests passed; additional probes confirmed the serialization change and mutation after a rejected owner merge.
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate changes to refund ownership and credit accounting in FeeRefunds::checked_add_assign and from_typed_storage_removal, plus storage-flag codecs and batch refund-cost propagation, directly changing funds-accounting machinery despite deferring protocol activation and final routing. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/util/storage_flags/combine.rs`:
- [BLOCKING] packages/rs-drive/src/util/storage_flags/combine.rs:74-81: Honor UseTheirs when single-epoch bucket flags shrink
The pinned crate's combine_with_higher_base_epoch_remove_bytes returns self immediately for SingleEpochOwned, before applying merging_owners_strategy. Mapping SingleEpochContractBucket to that variant therefore preserves the old bucket owner even when the typed update requests UseTheirs. I reproduced this through the generation-1 dispatcher: insert a 200-byte item with SingleEpochContractBucket(1, ...), replace it with the same payload and SingleEpochOwned(2, identity), then delete it. The two-byte-shorter identity header triggers the shrink path, and the delete records the original bucket rather than the identity. Generation 1 is inactive in shipped protocol tables, but its advertised cross-kind transfer behavior is already incorrect. Resolve typed ownership independently of this early return, leave the shipped v0 behavior unchanged, and add a dispatcher regression test for the later-epoch replacement.
In `packages/rs-dpp/src/fee/fee_result/refunds.rs`:
- [SUGGESTION] packages/rs-dpp/src/fee/fee_result/refunds.rs:46-50: Declare the FeeRefunds source and serialization break
The second tuple field changes both the public constructor and the derived codecs immediately, independently of Drive's protocol activation. A local probe confirmed that the new bincode decoder rejects the historical empty-map encoding, while Serde's empty representation changes from {} to [{},{}]. The PR's breaking-change section mentions the constructor change but not the serialization change, and the title lacks the required ! marker. Either preserve the legacy codec through a separate typed carrier, or mark the API change as breaking and document how users migrate both constructors and serialized values. No production persistence or wire consumer was established, so this is not a demonstrated consensus regression.
- [SUGGESTION] packages/rs-dpp/src/fee/fee_result/refunds.rs:203-211: Check owner collisions before merging refund credits
The credit loop mutates self.0 before this loop checks owner compatibility. A rejected merge of 10 identity-owned credits and 1 bucket-owned credit under the same carrier key leaves 11 credits associated with the original identity; a local probe confirmed that iter_typed subsequently accepts that state. For the colliding key, the problem is not a missing owner record but credits attributed to the retained, incompatible owner. Existing callers that discard the accumulator on error are safe, but reusing it after the rejected merge exposes that incorrect attribution. Preflight owner conflicts before changing either map and extend the collision test to assert that this error leaves the accumulator unchanged.
In `packages/rs-drive/src/util/storage_flags/codec.rs`:
- [SUGGESTION] packages/rs-drive/src/util/storage_flags/codec.rs:74: Avoid cloning the epoch map just to calculate its size
For historical MultiEpoch and MultiEpochOwned flags, this fallback constructs an owned crate value and clones the entire BTreeMap before computing its size. Before this PR, the re-export used the crate's borrowed size calculation directly. Existing document and index estimation paths call serialized_size, so these extra tree allocations affect shipped paths rather than only the inactive typed generation. Handle the historical multi-epoch variants using the existing borrowed epoch_map_size helper.
In `packages/rs-dpp/src/fee/refund_owner/mod.rs`:
- [NITPICK] packages/rs-dpp/src/fee/refund_owner/mod.rs:84-90: Build the fixed-size removal-key preimage without a heap allocation
The bucket preimage always contains exactly 69 bytes, but removal_key allocates a Vec on every call. The split and combine paths call this helper repeatedly. A stack buffer preserves the specified encoding while avoiding that allocation.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Legacy refund accessors use unwrap and wrapping arithmetic next to the new checked typed path — The cited legacy arithmetic and unwrap predate this PR. No new reachable input or caller was shown worsening those paths, and changing their historical semantics would be unrelated cleanup outside this review.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
… shrink later The crate's higher epoch removal combine returns the old flags untouched for single epoch inputs before it looks at the merging strategy, so a bucket owned item replaced by a smaller identity owned payload in a later epoch kept the bucket as its owner under the typed generation while a growing replace transferred. The typed combine now resolves the owner itself on that path with the crate's owner rule (no owner yields, equal keeps, different follows the strategy). A dispatcher test pins the later epoch shrinking replace end to end: the freed bytes refund the bucket, the stored flags name the identity, and the delete afterwards refunds the identity. Also from the review: FeeRefunds::checked_add_assign checks owner conflicts before either map changes so a rejected merge leaves the accumulator intact (test extended); the historical multi epoch variants serialize and size without cloning their epoch map; the bucket carrier key preimage is built on the stack. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 2 of the automated review addressed in b327473; ready for re-review.
Local gate (exit codes captured): 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at b327473 and confirmed that all five prior findings are addressed. Independent dispatcher probes reproduced two blocking defects in generation 1's cross-kind replacements; shipped protocol snapshots remain on generation 0 and are unaffected. The existing 117 targeted Drive tests, 37 DPP refund tests, verify-only Drive check, and whitespace check passed; temporary probes were removed and the working tree is clean.
🔴 2 blocking | 🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate storage-flag encoding and epoch-merging logic alongside changes to funds accounting in packages/rs-dpp/src/fee/fee_result/refunds.rs (FeeRefunds::from_typed_storage_removal and checked_add_assign), meeting both the complexity and critical-surface criteria even though protocol activation is deferred. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/util/storage_flags/update.rs`:
- [BLOCKING] packages/rs-drive/src/util/storage_flags/update.rs:67-70: Recalculate storage costs when cross-kind flags change width
GroveDB initially calculates an item replacement's storage cost using the old flags, but this helper returns false whenever the combined flags equal the proposed flags. For a same-epoch identity-to-bucket replacement, that equality holds even though the header grows from 35 to 37 bytes, so GroveDB skips the required recalculation. Independently reproduced through the generation-1 dispatcher: replacing a 200-byte SingleEpochOwned(1, id) item with a 201-byte SingleEpochContractBucket(1, contract, 7) item fails with StorageCostMismatch (added_bytes: 1, replaced_bytes: 361, actual_total_bytes: 364); the reverse ownership direction also fails. Report the size-affecting flag change even when merging leaves the proposed flags unchanged, and add dispatcher regressions in both directions. Generation 1 is inactive in shipped snapshots, but these valid replacements must work for the batch generation introduced here.
- [BLOCKING] packages/rs-drive/src/util/storage_flags/update.rs:58-61: Keep transferred ownership stable during cost recalculation
The SameSize branch can reverse an ownership transfer selected by the preceding cost iteration and prevent GroveDB's update loop from converging. Independently reproduced through the generation-1 dispatcher: insert a 200-byte item with SingleEpochOwned(1, id), then replace it with a 198-byte item carrying SingleEpochContractBucket(2, contract, 7). The initial shrink selects bucket ownership; its two extra header bytes cancel the payload shrink, so the next iteration is SameSize and restores the old identity flags here. That makes the item smaller again, and the iterations alternate until GroveDB returns `cyclic error updated value based on costs too many times`. Make ownership resolution consistent across recalculation when the net size reaches zero, and add a dispatcher regression for this boundary case without changing generation 0.
In `packages/rs-dpp/src/fee/fee_result/refunds.rs`:
- [SUGGESTION] packages/rs-dpp/src/fee/fee_result/refunds.rs:128-130: Enforce typed-owner safety at DPP's identity-only refund boundary
A valid bucket-bearing value returned by this constructor remains accepted by identity-only routing accessors, which ignore its recorded owner. An independent probe passed such a value through FeeResult::into_balance_change(payer).other_refunds() and received the bucket's carrier key as an identity recipient for the full refund. Drive's operation_cost() rejection does not protect direct DPP callers or the PreCalculatedFeeResult pass-through. Add a checked identity-only conversion at the DPP boundary and make identity-only balance/routing operations reject bucket-bearing refunds rather than reinterpret their keys. Cover the boundary with a valid from_typed_storage_removal value. This is a nonblocking safety gap in the newly exposed API, not a demonstrated shipped-protocol regression.
- [SUGGESTION] packages/rs-dpp/src/fee/fee_result/refunds.rs:183-194: Preserve missing-owner failures across refund merges
The collision preflight does not verify that each operand's credit keys have their own recorded owners. Consequently, merging can erase the missing-owner failure enforced by iter_typed: an independent probe combined 10 credits without an owner record and 1 bucket-owned credit under the same key, after which iter_typed accepted all 11 credits as belonging to the bucket. Both operand orders behave this way. The public tuple fields permit this incomplete state, so validate owner completeness for each operand before combining credits, or enforce that invariant at construction. Existing pricing constructors populate both maps, making this a nonblocking aggregation-safety issue rather than an established production regression.
In `packages/rs-drive/src/util/storage_flags/combine.rs`:
- [SUGGESTION] packages/rs-drive/src/util/storage_flags/combine.rs:166-170: Move owned epoch maps through the typed combine adapters
This function owns combined, but borrows and clones its epoch map into MultiEpochContractBucket immediately before dropping the original. Both combine methods likewise consume self and rhs while converting them through a borrowed adapter that clones their multi-epoch maps. Updates with epoch history therefore add avoidable linear tree copies and allocations around the delegated arithmetic. Destructure the owned crate enum to move its output map, and provide a consuming conversion for the input adapters; retain the borrowed adapter for callers that need to keep their inputs. This preserves the arithmetic and encoding while avoiding unnecessary work in the new batch generation.
… flag update GroveDB prices a replace with the old flags attached and re-prices only when the flag update closure reports a change. Two cases broke under the typed generation. A same-epoch replace across owner kinds leaves the proposed flags equal to the merged flags while the header grows or shrinks by two bytes, so the closure reported no change and GroveDB rejected the write with a storage cost mismatch. A later-epoch replace whose payload shrinks by exactly the header growth is priced as a shrink first and as same size on the second pass; the same-size branch restored the old flags, undoing the transfer the first pass chose, and the two passes alternated until GroveDB's cyclic guard fired. The typed closure now reports a change whenever the combined flags differ in width from the old flags, and its same-size branch keeps our epochs while resolving the owner by the same rule as the added and removed branches. The combine adapters move epoch maps instead of cloning them. Dispatcher tests pin both shapes in both directions. The shipped generation 0 closures are the crate's and unchanged. Also from the review: FeeRefunds::checked_add_assign refuses to merge credits that lack a recorded owner on either side, so a merge can never lend an owner to unowned credits; ensure_identity_owners_only guards the identity keyed accessors for new callers, and a Drive test pins that the shipped balance consumer halts on a bucket owned refund rather than crediting anyone. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 3 of the automated review addressed in 52b223c; ready for re-review.
Generation 0 closures remain the crate's, unchanged. Local gate (exit codes captured): 121 targeted Drive tests, 39 DPP refund tests, clippy with 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The complete diff preserves shipped batch behavior and leaves typed generation 1 inactive; the cross-kind replacement regressions are fixed. Three nonblocking findings remain: an owner-only refund merge can assign ownership to previously unattributed credits, cost aggregation discards typed-owner metadata, and the removal callback unnecessarily clones epoch maps. Independent validation passed 39 DPP refund tests, 120 targeted Drive tests, the Drive build, and the verify-only check; inspected API probes reproduced the aggregation issues, and the working tree remains clean.
🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff makes intricate changes to refund accounting in packages/rs-dpp/src/fee/fee_result/refunds.rs and storage-cost attribution in packages/rs-drive/src/util/storage_flags/{combine,split,update}.rs, directly changing how monetary refunds are calculated and attributed while preserving historical storage encodings. - Phase 1 reviewers: not run (skipped for throughput: 20 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/fees/op.rs`:
- [SUGGESTION] packages/rs-drive/src/fees/op.rs:357-362: Preserve refund-owner metadata when aggregating typed costs
combine_cost_operations accepts CalculatedCostOperationWithRefundOwners but returns only OperationCost, discarding the metadata that operation_cost deliberately refuses to erase. An independently executed probe using a generation-1 bucket deletion confirmed that direct consume_to_fees_v0 decoding rejects the operation, whereas combining it and wrapping the result in CalculatedCostOperation succeeds and records Identity(bucket_carrier_key). Rewrapping the aggregate is already the pattern in apply_contract_with_serialization_v0, although its current inputs are read costs and do not demonstrate production misrouting. Provide an owner-preserving aggregate with collision checks, or reject typed inputs from the legacy combiner and expose any accounting-only projection separately with an explicit contract. This addresses the newly introduced API's metadata loss without activating typed routing.
In `packages/rs-drive/src/util/storage_flags/split.rs`:
- [SUGGESTION] packages/rs-drive/src/util/storage_flags/split.rs:53-54: Consume decoded flags when splitting removal bytes
This callback owns the freshly decoded storage_flags and does not use it after splitting. The borrowed split_storage_removed_bytes adapter nevertheless calls to_crate_flags_keyed_by_removal_key, cloning the complete BTreeMap for every multi-epoch variant before dropping the decoded original. Generation-1 deletions and shrinking replacements therefore add an unnecessary linear tree copy and allocations, potentially repeatedly during cost recalculation. Use the existing consuming adapter here; the public borrowed helper can remain available for callers that retain their flags.
In `packages/rs-dpp/src/fee/fee_result/refunds.rs`:
- [SUGGESTION] packages/rs-dpp/src/fee/fee_result/refunds.rs:210-220: Preserve missing-owner failures across refund merges
(existing thread: https://github.com/dashpay/platform/pull/4789#discussion_r4033897664)
The preflight now rejects both reported credit-bearing operand orders, but an owner-only RHS bypasses it because the missing-owner check visits only rhs_credits.keys(). Independently reproduced: merge FeeRefunds({K: {0: 10}}, {}) with FeeRefunds({}, {K: bucket}), using the bucket's removal key for K. checked_add_assign returns Ok, and iter_typed changes from a missing-owner error to attributing all 10 credits to the bucket after self.1.extend(rhs_owners). This violates the explicit invariant that existing unattributed credits must not acquire ownership from the other operand. Check incoming owner-map keys against unowned LHS credits before either map changes, and add a metadata-only regression asserting rejection without mutation. Pricing constructors populate both maps consistently, so this is public-API robustness rather than a demonstrated shipped-protocol regression.
combine_cost_operations folded typed cost operations into a plain OperationCost, which erased the recorded refund owners that operation_cost refuses to drop; a caller rewrapping the aggregate as a CalculatedCostOperation could then have the shipped decoder price a bucket's carrier key as an identity. The plain combiner now folds only plain costs, as it did before this branch, and a new combine_cost_operations_with_refund_owners aggregates both kinds while merging owner maps with collision detection. Also from the review: the typed split callback consumes its decoded flags instead of cloning the epoch map, and FeeRefunds::checked_add_assign refuses an owner record that arrives without credits when the accumulator already holds those credits unowned. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 4 of the automated review addressed in 2392dad (all three suggestions, none blocking).
Local gate (exit codes captured): 123 targeted Drive tests, 39 DPP refund tests, clippy with 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Verified the complete diff at 2392dad and reconciled all 12 prior findings: 11 are fixed, and mandatory typed routing remains intentionally deferred to part 2. Two nonblocking suggestions remain: incomplete owner attribution in the new cost aggregator and missing StorageFlags API migration documentation; no blocking regression was established. Independent validation passed 123 targeted Drive tests, 39 DPP refund tests, two temporary attribution probes, and the verify-only Drive check; the worktree is unchanged.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate, cross-cutting funds-accounting changes in packages/rs-dpp/src/fee/fee_result/refunds.rs (typed refund attribution and checked_add_assign) and packages/rs-drive/src/fees/op.rs (typed removal-cost processing), alongside new storage-flag encoding and batch-apply logic, even though protocol activation is deferred. - Phase 1 reviewers: not run (skipped for throughput: 22 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/fees/op.rs`:
- [SUGGESTION] packages/rs-drive/src/fees/op.rs:383-390: Establish each input's refund owners before aggregating costs
The plain arm adds sectioned removal bytes without recording their implicit identity owners. I independently confirmed that combining a plain identity removal with a typed bucket removal returns both removal keys but only the bucket owner, causing FeeRefunds::from_typed_storage_removal to reject the aggregate. When the plain identity key equals the bucket carrier key, aggregation instead succeeds and attributes both inputs to the bucket, bypassing the intended collision check. The typed arm has the same provenance problem for incomplete inputs: 100 bytes with an empty owner map plus 40 attributed bytes under the same key become 140 accepted bucket-owned bytes. Before merging costs, normalize every non-system section key in a plain input to RefundOwner::Identity and check owner conflicts, or explicitly reject plain sectioned removals; also require each typed input to supply its own owner records for its non-system removal keys. Add tests containing actual sectioned removals, since the current successful aggregation test uses NoStorageRemoval. This is a correctness issue in the new public helper, not a demonstrated shipped-protocol regression: it currently has no production callers.
In `packages/rs-drive/src/util/storage_flags/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/util/storage_flags/mod.rs:65-72: Document the StorageFlags source-compatibility migration
Replacing the public re-export with a distinct Drive enum introduces another Rust API break beyond those listed in the breaking-changes section. Drive's StorageFlags is no longer interchangeable with grovedb_epoch_based_storage_flags::StorageFlags, and the previously public deserialize_single_epoch, deserialize_multi_epoch, deserialize_single_epoch_owned, and deserialize_multi_epoch_owned methods are absent from the replacement. These callers stop compiling independently of protocol activation. Document the migration alongside the FeeRefunds and LowLevelDriveOperation changes: the existing From implementation converts historical crate values into Drive flags, and the general deserialize entry point replaces the specialized parsing entry points where appropriate. This does not require changing historical protocol tables.
combine_cost_operations_with_refund_owners merged owner maps without looking at the removals they described. A plain cost's sectioned identity keys were left unattributed, so an aggregate of a plain identity removal and a typed bucket removal could not be priced by from_typed_storage_removal, and a plain key equal to a bucket carrier key was silently attributed to the bucket. A typed input could likewise contribute bytes under a key it never recorded and have another input's owner cover them. The aggregate now establishes the owner of every sectioned key per input before anything merges: a plain cost's keys are identities, as its identity only flags say; a typed cost must have recorded an owner for each of its own keys or the aggregate fails; the system key is never an owner; one key attributed to two owners within or across inputs is refused. Tests use real sectioned removals, including pricing the aggregate through the typed constructor. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 5 of the automated review addressed in ce89086 (two suggestions, none blocking).
Local gate (exit codes captured): 123 targeted Drive tests, 39 DPP refund tests, clippy with 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Part 1 typed-refund-owner plumbing is consensus-inert and prior work verifies fixed; one minor inconsistency remains in the new combiner where the owner-map merge loop does not skip the system key like the two removal-key loops above it. No blockers, no version-table action required at this head.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Large additive change adding RefundOwner, typed StorageFlags variants, and typed fee-refund paths, but part 1 keeps shipped generations byte-identical with no active routing or migration so it does not change live consensus or funds movement. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 14% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — platform-versioning (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — platform-versioning (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/fees/op.rs`:
- [SUGGESTION] packages/rs-drive/src/fees/op.rs:430-432: Typed cost combiner copies the system key from the owner map
combine_cost_operations_with_refund_owners documents the system key is never an owner and both removal-key loops skip SYSTEM_REFUND_CARRIER_KEY, but the third loop merging each typed input's refund_owners map has no such skip. The v1 split closure can never produce a system-key entry (zero identity records nothing, bucket-with-system-key fails), so this is reachable only via a manually constructed CalculatedCostOperationWithRefundOwners — yet recording a system-key owner contradicts the documented invariant and would confuse part 2 consumers that treat every owners-map entry as routable. Skip it for consistency.
The typed cost aggregate skipped the system carrier key when attributing sectioned removals but not when merging an input's recorded owner map, so a hand-built operation could hand a consumer a system-key owner that is never routable. The merge loop now skips it like the two loops above it; a test pins that a system-key entry in an owner map records nothing. Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Part 1 typed-refund staging is sound at this head: both shipped v0 generations stay pinned to the crate flags type, v1 records typed owners fail-closed, and the final delta closes the system-key owner-map gap. All 15 prior findings verify as fixed in code and tests; the two remaining fresh suggestions are pre-existing or redesign asks outside this PR, so no in-scope findings remain.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Large additive typed-refund scaffolding across rs-dpp/rs-drive but part 1 keeps shipped generations byte-identical with no typed-flag writes or routing yet, so it does not itself change consensus or funds movement. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 14% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — platform-versioning (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — rust-quality (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — platform-versioning (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — rust-quality (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Part 2 must activate the staged v1 generations — The v1 batch-apply generations and typed refund routing are fully staged but unreachable: no live version snapshot selects generation 1 and no shipped path writes or decodes bucket flags. This is the intended part-1 posture.
- Follow-up: In FIX-08 part 2, add the grove apply version slots to the new PlatformVersion/DriveVersion snapshot, gate bucket-flag writes and typed routing on the activation version, and add replay coverage showing pre-activation blocks still execute v0.
- FeeRefunds pub tuple fields permit invariant-violating construction — Dropped as broader redesign outside this PR's scope — the pub tuple-field shape predates this PR (single pub field) and the PR extends that established pattern; the invariant is enforced at merge, pricing, and iteration as designed, and privatizing construction would expand the Rust API break further.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
PR Hygiene: the checklist is in the description. |
|
/self-reviewed c96cd43 |
Issue being fixed or feature implemented
Part of the smart contract plan in #4626 (task FIX-08, fees workstream #4689).
Storage refunds today are keyed by a bare 32-byte identifier that every consumer reads as an identity. Contract credit buckets will also pay for storage (contract calls, receipts, jobs), so a refund must know the kind of its owner without inferring it from the shape of an id. The confirmed policy asks for a typed refund owner recorded when the bytes are stored, historical records decoding exactly as before, refunds routed to identities and to contract buckets on their own paths, and wiped-owner refunds going to the processing pool.
This is part 1 of 2 for FIX-08. It lays down the typed owner, the typed refund carrier, the typed storage flags and the batch apply generation that records owners. No version table changes, every shipped generation stays byte-identical, and no path writes or decodes typed flags yet. Part 2 (gated on the 5.0 protocol version, after the fee history decoder, the storage refund primitive and the contract credit buckets land on
v5.0-dev) adds the decode generation, the routing primitive and the table amendments.What was done?
rs-dpp
packages/rs-dpp/src/fee/refund_owner/mod.rs(new):RefundOwner { Identity(Identifier), ContractBucket { contract_id, position } }withEncode,Decode,Serialize,Deserialize,Copy,Ord,Hash.removal_key()gives every owner one 32-byte carrier key: an identity's key is its id verbatim; a bucket's key ishash_double(domain || contract_id || position_be)with the 35-byte domaindash-platform/refund-owner/bucket/0, 69 bytes of preimage with no length prefixes.as_identity(),SYSTEM_REFUND_CARRIER_KEY,RefundOwnersByIdentifier, and a localContractCreditBucketPosition = u16alias (the contract credit bucket work defines the same width; the alias keeps this PR independent of merge order).packages/rs-dpp/src/fee/fee_result/refunds.rs:FeeRefundsgains a second tuple field recording the typed owner of every carrier key.from_storage_removalkeeps its signature and recordsIdentityfor every key explicitly. Newfrom_typed_storage_removalreads the recorded map and returnsCorruptedCodeExecutionon an unmapped key.checked_add_assignmerges owners and rejects one key recorded for two owners. Newowner_of,iter_typed,calculate_all_refunds_except_owner. Existing accessors keep their signatures.packages/rs-dpp/src/fee/fee_result/mod.rs:BalanceChangeForIdentity::other_typed_refunds()alongsideother_refunds().FeeRefundsgain the second field.rs-drive
packages/rs-drive/src/util/storage_flags/{mod,codec,combine,split,update,tests}.rs: the three-line re-export becomes Drive's ownStorageFlags. The four historical variants keep their names and shapes; their bytes are produced and parsed by the pinnedgrovedb-epoch-based-storage-flagscrate so they cannot drift. Two new variants:SingleEpochContractBucket(epoch, contract_id, position)(type byte 4, 37 bytes) andMultiEpochContractBucket(epoch, epochs, contract_id, position)(type byte 5). The parse helpers decode all six types so the shipped readers carry bucket flags through. Newupdate_element_flags_typed,split_removal_bytes_typed(sections underremoval_key()and records the owner, refusing one key with two owners or a bucket key equal to the system key),refund_owner(),new_single_epoch_for_owner,approximate_size_for_owner. Combine maps typed owners to their carrier keys for the crate's epoch arithmetic and maps the winner back through the two inputs, soUseTheirstransfers ownership across kinds andRaiseIssuerefuses. The eight direct crate imports switch tocrate::util::storage_flags::StorageFlags.packages/rs-drive/src/fees/op.rs:LowLevelDriveOperation::CalculatedCostOperationWithRefundOwners { cost, refund_owners }appended.operation_cost()rejects it withCorruptedCodeExecution, soconsume_to_fees_v0(and any future decoder that reachesoperation_costthrough its catch-all arm) fails closed on it.combine_cost_operationsleaves it out, since folding it into a plain cost would erase the owners; the newcombine_cost_operations_with_refund_ownersaggregates plain and typed costs together and returns the owner of every sectioned removal key: a plain cost's keys are identities (its flags were identity-only), a typed cost must carry a recorded owner for each of its own sectioned keys, the system key is never an owner (skipped both in the removals and in a recorded owner map), and one key attributed to two owners within or across inputs is refused.packages/rs-drive/src/util/grove_operations/mod.rs:push_drive_operation_result_with_refund_owners.grove_apply_batch_with_add_costs/v1andgrove_apply_partial_batch_with_add_costs/v1with their1 =>arms: copies of v0 switched to the typed closures, pushing the typed operation. Noplatform-versionchange: a grove table generation referenced by noPLATFORM_V*is what the conventions forbid, so the new grove table arrives with part 2. The tests select version 1 through a test-builtDriveVersion.Book:
book/src/fees/overview.md(Refunds) andbook/src/drive/cost-tracking.mddescribe the typed owner, the carrier key rule and the typed cost operation.Shipped generations stay bound to the crate type. Replacing the re-export with a Drive type would have silently rebound both shipped batch apply generations to the new type through their unchanged
use crate::util::storage_flags::StorageFlagsline. The full batch v0 would still have failed closed, but the partial batch v0 inlines its closures over the parse helpers, which decode all six types, so over bucket flags it would have sectioned the bytes under the carrier key without a recorded owner and the shipped fee decoder would have read that key as an identity. Both v0 modules therefore importgrovedb_epoch_based_storage_flags::StorageFlagsdirectly: the one import line is the exact type the shipped code was compiled against before this PR, so both generations are behaviour-identical to what shipped and reject type bytes 4 and 5 through the crate's decoder. The Drive type carries no shipped-name closure wrappers. Dispatcher tests on both methods pin that a bucket-owned delete under generation 0 errors, applies nothing and prices no removal (only the read cost incurred before the failure is pushed, as for any failed grove operation), and that generation 1 then removes the item with the recorded owner. (Found by the independent review of round 1.)How Has This Been Tested?
New tests:
rs-dpprefund owner: identity key verbatim, pinned bucket vector (contract id0x11times 32, position 7 givese0cb6d5af10d1ece2cf9dccc39dac999ce9cd59e8c1314bc7017f19cfb9e5a1d), the 69-byte preimage, distinct keys for distinct buckets and contracts, bincode round trip of both kinds.rs-dppfee refunds:from_storage_removalrecords identities for every key;from_typed_storage_removalrecords from the map, prices exactly as the untyped path and rejects an unmapped key;checked_add_assignmerges owners and rejects one key with two owners;calculate_all_refunds_except_ownersums per typed owner and skips the payer;iter_typedfails closed on an unrecorded key;sum_per_epochacross both kinds.rs-drivestorage flags: round trip of every variant; types 0 to 3 byte-equal to the crate; pinned byte layouts of all six types (including the 35-byte owned single epoch and the 37-byte bucket single epoch); unknown type byte 6 and 255 rejected; truncated, oversized and dangling bucket flags rejected;serialized_sizeequalsserialize().len();approximate_size_for_owner; typed owner accessors. Split: bucket delete sections under the carrier key and records the owner; multi-epoch bucket shrink takes from the latest epochs first (equal to the crate's LIFO for an identity); identity recorded; unowned under the system key with nothing recorded; the all-zero identity stays system bytes; one key with two owners rejected within a batch; the crate closure entry points the shipped generations use reject bucket flags and still serve identity flags. Combine: same-epoch bucket keeps its owner;UseTheirstransfers identity to bucket and back;RaiseIssueerrors across kinds and across buckets; an identity whose id equals a bucket's carrier key never merges; higher-epoch add and remove keep the typed owner and collapse correctly; newer base epoch into older rejected. Update: identical to the crate for identity flags; bucket growth, transfer, shrink, same-size and insert pass-through; removing flags rejected.rs-drivebalance consumer: the shippedapply_balance_change_from_fee_to_identityhalts withCorruptedCodeExecutionon a bucket-owned refund.rs-drivefee ops:operation_costrejects the typed operation;consume_to_fees_v0fails closed on a typed operation;combine_cost_operationsleaves it out;combine_cost_operations_with_refund_ownersattributes every sectioned key of plain and typed inputs (tested with real sectioned removals: plain identity plus typed bucket aggregate and price throughfrom_typed_storage_removal; a typed input sectioning bytes under an unrecorded key is rejected; a plain key equal to a bucket carrier key and two typed records for one key are both rejected).rs-drivebatch apply, through the dispatcher with a test-built drive version at 1: the same identity-owned item deleted under v0 and v1 yields the same root hash and the same cost (only the operation shape differs); a bucket-owned delete under v1 yields the typed operation with the recorded owner; v0 rejects bucket flags, applies nothing and prices no removal, and v1 then removes the item with the recorded owner; a same-epoch replace across kinds in both directions applies and re-prices for the header width change; a later-epoch replace that nets to the same size across kinds converges and transfers; an unknown generation is rejected. The partial batch has the identity equality test, the v1 bucket test and the same v0 fail-closed test.Commands run locally (exit codes captured to files, all zero):
Breaking Changes
None at any protocol version. No version table changes; every shipped generation is byte-identical; storage flag types 0 to 3 encode and decode exactly as before; no path writes or decodes types 4 and 5 yet.
Rust API, not consensus:
FeeRefundsgains a second public tuple field, so its tuple constructor takes two maps (eight constructor sites, all tests, updated in this PR) and its derived bincode and serde encodings change shape (the empty value moves from one map to a pair of maps; the old bincode bytes no longer decode). Nothing persists or transmits aFeeRefunds:BlockFeesstores the per-epoch sum,PlatformStateForSavingstores fee versions only, no proto message carries it and no crate outsidedpp,driveanddrive-abcinames it, so there is no stored or wire value to migrate. Callers that construct it directly pass the owners map alongside the credits map, or usefrom_storage_removal, which records identities itself.LowLevelDriveOperationgains a variant (no exhaustive match outsideop.rs).drive::util::storage_flags::StorageFlagsis now Drive's own enum rather than a re-export ofgrovedb_epoch_based_storage_flags::StorageFlags, so the two types are no longer interchangeable. Migration for a caller that held a crate value:StorageFlags::from(crate_value)converts the four historical variants; the crate's specialised entry points (deserialize_single_epoch,deserialize_multi_epoch,deserialize_single_epoch_owned,deserialize_multi_epoch_owned) are not carried over, andStorageFlags::deserialize(orfrom_element_flags_ref) replaces them, dispatching on the type byte. Every other method keeps its name and signature. Inside this repository no caller used the specialised decoders and the eight direct crate imports were switched in this PR; the shipped batch apply generations import the crate type directly on purpose (see above).Decisions taken (provisional values)
hash_double(b"dash-platform/refund-owner/bucket/0" || contract_id (32) || position (2, big-endian)), 69 bytes of preimage, no length prefixes; the trailing/0is the derivation generation and a future change is a new type byte. Pinned vector in the DPP test. Proposed as the allocation for the owner to confirm; provisional in status, exact in specification. The plan review left one open finding on this point (the encoding is underspecified in the register and still provisional); the specification above is the implementer's disposition.RefundOwneris not wrapped in a$formatVersionenum: DPP never persists it (storage persists it through the flag type byte, which is its version) and its wire use is inside consensus-internal fee results. Provisional.ContractCreditBucketPositionis a localu16alias underfee::refund_owneruntil the contract credit bucket module lands; same width either way.UseTheirsrule, so a replace whose new flags name a bucket moves the bytes to the bucket and back. The typed update closure differs from the crate's in three places, each forced by how GroveDB prices a replace (with the old flags attached, re-pricing only when the closure reports a change): a later-epoch shrink of single-epoch flags resolves the owner itself instead of returning the old flags; a change of header width (35 bytes identity, 37 bytes bucket) is reported as a change even when the merged flags equal the proposed ones, so GroveDB re-prices instead of rejecting the write; and a same-size pass keeps our epochs but resolves the owner by the same rule as the other branches, so a replace first priced as a shrink and then as same size converges. All three shapes are pinned by dispatcher tests in both directions (found by the automated review).FeeRefunds::checked_add_assignrefuses a right-hand side whose credits lack a recorded owner and refuses to lend an owner to credits the left-hand side holds unowned, whether the owner arrives with credits or on its own; the check runs before either map changes.ensure_identity_owners_only()guards the identity-keyed accessors for new callers; the shipped balance consumer keeps calling them unguarded because it runs only under generations that predate typed owners, and a Drive test pins that it halts on a bucket-owned refund rather than crediting anyone.checked_add_assignandfrom_typed_storage_removalfail the same way. Every node computes the same key from the same bytes, so a collision halts every node on the same block.Merge-order note: the fee history decoder work on
v4.3-deveditsfrom_storage_removalin place and addsconsume_to_fees_v1toop.rs; this PR touches the same function body and the enum region. Whichever lands second resolves a small textual conflict with no semantic choice.Part 1 of 2 for FIX-08.
Refs #4689
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Automated reviewer consensus (Fable 5.1 implementer, GPT-6 Astra reviewer)
Reviewer consensus
Plan Review consensus
resolvedresolvedstill_openresolvedReview consensus
resolvedPR Hygiene ·
c96cd43book/src/drive/cost-tracking.md,book/src/fees/overview.md) — QuantumExplorer or shumkovdpp(packages/rs-dpp/src/fee/fee_result/mod.rs,packages/rs-dpp/src/fee/fee_result/refunds.rs,packages/rs-dpp/src/fee/mod.rsand 1 more) — QuantumExplorer or shumkovrs-drive(packages/rs-drive/src/drive/contract/get_fetch/fetch_contract_ids/v0/mod.rs,packages/rs-drive/src/drive/contract/get_fetch/fetch_contracts/v0/mod.rs,packages/rs-drive/src/drive/group/insert/add_group_action/v0/mod.rsand 23 more) — QuantumExplorer or shumkovWhen every box is checked the
PR Hygienecheck passes and this can merge.